Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Nutrition Game #70 #114

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from
Draft

Conversation

Shmigolk
Copy link
Collaborator

@Shmigolk Shmigolk commented Aug 1, 2022

I added new page nutrition and made simple UI. I need your recommendation about UI, logic, etc.
image

@VaiTon
Copy link
Member

VaiTon commented Aug 1, 2022

I would add some basic infos about the product that is being edited, like name, brand, size, just to make sure that the info actually makes sense

@VaiTon VaiTon linked an issue Aug 1, 2022 that may be closed by this pull request
Copy link
Collaborator

@nobeeakon nobeeakon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work, just added some comments, please don't hesitate to ask if you have any question

src/i18n/common.json Outdated Show resolved Hide resolved
src/i18n/en.json Outdated Show resolved Hide resolved
src/pages/nutrition/additionalNutritions.jsx Outdated Show resolved Hide resolved
return (
<div>
<FormControl sx={{ m: 1, minWidth: 80, margin: '0' }}>
<InputLabel id="demo-simple-select-autowidth-label">Unit</InputLabel>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these ids could be improved, what about something like 'dropdown-unit-label' ?... better to use something as specific as we can or we could end up having multiple similar ids

<MenuItem value="">
<em>None</em>
</MenuItem>
<MenuItem value={10}>g</MenuItem>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think it would be nice to somewhere something that gives you an idea of what these values (10, 21, 22) actually mean. What about using an object outside of this component, something like const weightUnits = {g:10, mg:21, kg:22} (please note that I don't really understand what those values refer to and where do they come from). It is better to have some auto documentation rather than having some unkown values

src/components/ResponsiveAppBar.jsx Outdated Show resolved Hide resolved
package.json Outdated
@@ -19,6 +21,7 @@
"react-medium-image-zoom": "^4.4.3",
"react-router-dom": "^6.3.0",
"react-scripts": "5.0.1",
"react-virtualized": "^9.22.3",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this being used somewhere?

package.json Outdated
"@mui/icons-material": "^5.8.4",
"@mui/lab": "^5.0.0-alpha.89",
"@mui/material": "^5.8.7",
"@mui/x-data-grid": "^5.13.0",
"axios": "^0.27.2",
"clsx": "^1.2.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this package used?

package.json Outdated
@@ -6,11 +6,13 @@
"dependencies": {
"@emotion/react": "^11.9.0",
"@emotion/styled": "^11.8.1",
"@material-ui/system": "^4.12.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it useful to have this package? I see that it is kind of used in pages/nutrition/index.jsx but seems to be importing a non used flexbox (what would be the advantage of using that flexbox compared to css defined flexbox?)

name={nutrition.label}
onChange={onchangeHandler}
/> , <SelectAutoWidth /></Box>,
<Checkbox sx={{}} />));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is sx={{}} needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed checkbox and leaved delete icon. Still working on it

@VaiTon VaiTon requested a review from nobeeakon August 1, 2022 20:13
@teolemon teolemon changed the title Nutrition Game #70 feat: Nutrition Game #70 Aug 2, 2022
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds already quite solid and well structured. I did not had time to have a look to the management of the table itself

src/components/ResponsiveAppBar.jsx Outdated Show resolved Hide resolved
src/pages/nutrition/productCard.jsx Outdated Show resolved Hide resolved
const productListUrl = getNutritionToFillUrl({page})
fetch(productListUrl)
.then(res => res.json())
.then(data => setProducts(data.products))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe set the index to 0, or keep the previous products with setPoducts(prevProducts => [...prevProducts, ...data.products])

Comment on lines 144 to 145
setNutriments={setNutriments}
setAdditionalNutriments={setAdditionalNutriments}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are trying to keep up to date 2 different arrays which is error pron. As soon as you forget an edge case, the two arrays will not be sync and it's hard to debug

I propose to keep the setNutriments that could be renamed addNutrimentToTheTable
Replace the setAdditionalNutriments by missingNutriments which would be computed as follow:

const missingNutriments = offNutriments.filter(nutrimentKey => 
	data.every(element) => element.off_nutriment_id !== nutrimentKey) 
)

with offNutriments the list of all the nutrients available on openfoodfact

This would replace options and setAdditionalNutriments

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can start by removing the @material-ui/system, react-virtualized, and clsx libraries.

And focus on the management of table rows

src/components/ResponsiveAppBar.jsx Outdated Show resolved Hide resolved
import NutritionTable from "./table";
import ProductNutriments from "./productCard";
import { Box } from "@mui/material";
import { flexbox } from "@material-ui/system";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { flexbox } from "@material-ui/system";

imported but not used. Normally when you do yarn start in the console, you should see some warnings about lines where you import or define variables you do not use. You should remove them to get a cleaner code.


Here is what I get when I run yarn start from your branch

image

Comment on lines 18 to 25
{
off_nutriment_id: "energy_kcal",
label: "Shugar",
value: "",
unit: "null",
quantification: "<",
robotoffPrediction: null
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated

Suggested change
{
off_nutriment_id: "energy_kcal",
label: "Shugar",
value: "",
unit: "null",
quantification: "<",
robotoffPrediction: null
},

@@ -0,0 +1,55 @@
const basicNutriments = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename it defaultNutrimentTableData Because it's the default values that will be edited by the user

off_nutriment_id: "energy_kcal",
label: "Protein",
value: "",
unit: "null",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a way to say "there is no unit specified for this nutriment". It can be either null or an empty string, but not the string "null"

Suggested change
unit: "null",
unit: "",
Suggested change
unit: "null",
unit: null,


export default function ProductNutriments({setNutriments}) {

const { getNutritionToFillUrl } = offService
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, but writing offService.getNutritionToFillUrl() as an advantage. When reading the line of code we know this data is fetched from openfoodfact server. Otherwise it could be fetched from robotoff, or an other file

Suggested change
const { getNutritionToFillUrl } = offService


React.useEffect(() => {
console.log('fetched')
const productListUrl = getNutritionToFillUrl({page})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const productListUrl = getNutritionToFillUrl({page})
const productListUrl = offService.getNutritionToFillUrl({page})

value={nutrition.value}
name={nutrition.label}
onChange={onchangeHandler}
/> , <SelectAutoWidth /></Box>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it's a mistake. The </Box> is planned to be before the coma But that's not important.

@Shmigolk I think you get confused by the MUI demo, because it's cells only contain strings/numbers, so they use

<TableCell>{row.thValueToDisplay}</TabCell>

In our case, we want to render inputs which is a bit more complex.

I propose you the following strategy:
create a component NutritionTableRow as follow:

const NutritionTableRow = ({ nutrimentData, updateValue, updateUnit, updateQuantificator }) => {
	const onNutrimentValueChange = (event) => {updateValue(event.target.value)}
	const onUnitChange = () => {...}
	const onQuantificatorChange = () => {...}
	
	const {label, value, quantification, unit} = nutrimentData
	
	return <TableRow>
		<TableCell>
			<TextField select value={quantification} onChange={onQuantificatorChange}>
				<MenuItem value="=">=</MenuItem>
				<MenuItem value=">">></MenuItem>
				<MenuItem value="<"><</MenuItem>
			</TextField>
		</TableCell>
		<TableCell>
			<TextField label={label} value={value} onChange={onNutrimentValueChange} />
		</TableCell>
		//  Same for the unit
	<TableRow>
}

Then you will be able to simplify your table content as follow:

	nutriments.map(nutriment => <NutritionTableRow nutrimentData={nutriment}  updateValue={updateUnit(nutriment.off_nutriment_id)}/>)

The trickiest part being updateValue which is a function returning a function.

const updateValue = (nutrimentId) => (newValue) => {
	// We get the index of the nutriment we want to update
	const indexToModify = nutriments.findIndex(nutriment => nutriment.off_nutriment_id === nutrimentId)
	
	// early return if not element correspond to this id
	if(indexToModify < 0){ return }
	
	// We update the value to the nutriment
	const newNutriment = {...nutriments[indexToModify], value: newValue }
	setNutriments([...nutriments.slice(0, indexToModify), newNutriment, ...nutriments.slice(indexToModify+1)])
}

Comment on lines 15 to 32
<div>
<FormControl sx={{ m: 1, minWidth: 80, margin: '0' }}>
<InputLabel id="demo-simple-select-autowidth-label">Unit</InputLabel>
<Select
labelId="demo-simple-select-autowidth-label"
id="demo-simple-select-autowidth"
value={age}
onChange={handleChange}
autoWidth
label="Age"
>
<MenuItem value="">
<em>None</em>
</MenuItem>
<MenuItem value={10}>g</MenuItem>
</Select>
</FormControl>
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a shortcut for that by using TextField. It's

<TextField select value={...} onChange={...} label="unit">
	<MenuItem value={null}></MenuItem>
	<MenuItem value="g">g</MenuItem>
	<MenuItem value="mg">mg</MenuItem>
	...
</TextField>

return { label, property, unit };
}

export default function NutritionTable({nutriments, setNutriments, additionalNutriments, deleteItem, setAdditionalNutriments, onchangeHandler}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally with those props it should be enough. The page does not need to know what you're doing. it just care about having the correct data thanks to setNutriments

Suggested change
export default function NutritionTable({nutriments, setNutriments, additionalNutriments, deleteItem, setAdditionalNutriments, onchangeHandler}) {
export default function NutritionTable({nutriments, setNutriments }) {

@teolemon
Copy link
Member

teolemon commented Aug 11, 2022

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like the previous review I did has not been saved.

id="nutrition-input"
options={options}
sx={{ width: 245, marginLeft: 2, marginTop: 2 }}
onChange={event => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we discussed that you can use newValue to avoid some edge cases

Suggested change
onChange={event => {
onChange={(event, newValue) => {

}

return (
<Box display={"flex"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Box display={"flex"}
<Box display="flex"

Some points which will be useful for tech interview about React (JSX syntax to be precise):

  • By default props are provided as follow propName={propValue}
  • You do not need the {} for sting. FOr example propName={"some text"} is same as propName="some text"
  • A prop alone is equivalent to true. For example <Component isOpen /> is the same as <Component isOpen={true} />.

@nobeeakon
Copy link
Collaborator

Sounds this PR is increasing its size, I know it has been a lot of time invested on this, but I wonder if we could split what has been done here in smaller PRs with a limited and concrete objective each?, so that those changes can be reviewed and merged in a quicker way. I think #71 can be a good starting point. So, basically some of the things you already have in this PR @Shmigolk , but trying to tackle them in an step by step approach as this PR already has a lot of comments and is covering a number of things and features so more space for comments and becomes more time consuming to review. What do you guys think @Shmigolk , @alexfauquette ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Nutrition Game (Tracker)
5 participants